Jedis 4.0.0 #698
Jedis 4.0.0 #698
Conversation
tbradellis
force-pushed
the
jedis-4.0.0
branch
from
872a232
to
4d9df06
Compare
|
This is ready for review. I have one question concerning a new method. In this new method there is a Example: |
| @NewField | ||
| private String host; | ||
|
|
||
| @NewField | ||
| private int port; | ||
|
|
||
| @NewField | ||
| private HostAndPort hostAndPort; |
There was a problem hiding this comment.
Could you do like the old instrumentation and call getHostAndPort() when you need the info.
I worry that this will add an increase in memory due to the new classes that will be created for these fields and the maps they require.
The call to getHostAndPort is pretty straightforward and shouldn't add much overhead. Or it should be comparable with the map lookup that is needed with the (a)NewField.
And as an added benefit, you instrument one less method.
| } | ||
|
|
||
| private void reportMethodAsExternal(ProtocolCommand command, String serverUsed, int serverPortUsed) { | ||
| String operation = "unknown"; |
There was a problem hiding this comment.
Also, the call to getHostAndPort could be done inside this method.
instrumentation/jedis-4.0.0/src/main/java/redis/clients/jedis/Connection_Instrumentation.java
Show resolved
Hide resolved
| @NewField | ||
| private int port; | ||
|
|
||
| @Trace |
There was a problem hiding this comment.
Any reason to trace this new method?
It may cause confusion when people upgrade Jedis and see a new span in their traces.
|
@aonuki I noticed that. maybe there is an opportunity in this JedisPubSub_Instrumentation class to also drop some @newfield usage and save a little memory (like in the Connection_Instrumentation). The only thing I see that might make a difference is the number of calls I’d need to make to get the host and port in each of the methods. Do you think this would be a good change here or would leaving it in this case make sense for any reason? Readability or anything else? |
|
In JedisPubSub, you could move the call to inside the |
|
Oh, derp...yes that could be done the same too. Thanks!! |
This adds instrumentation for Jedis 4.0.0+)
Maintains the same instrumentation points as previous versions. JedisPubSub and Connection instrumentation.
Related Github Issue
#641
https://github.com/redis/jedis/tree/4.0
Testing
Uses verifyInstrumentation in the usual way.
Additional integration test with local apps to ensure correct counts and no loss of instrumentation features.
Checks
[ NA] Are your contributions backwards compatible with relevant frameworks and APIs?
[ ] Does your code contain any breaking changes? Please describe.
[ ] Does your code introduce any new dependencies? Please describe.